Test: Add local test schema store - #152
Draft
ClausHolbechArista wants to merge 1 commit into
Draft
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR replaces the remote test schema download mechanism with local generation from YAML files, adds three comprehensive test schemas for validation feature coverage, integrates the generated artifact into the CI release workflow, and updates the test suite to use the new schemas. ChangesLocal Test Schema Store Generation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/test_schema_store/src/bin/build_test_schemas.rs`:
- Line 8: Add a brief comment above the standalone `use avdschema as _;` in
build_test_schemas.rs explaining it is present only to satisfy
`#![deny(unused_crate_dependencies)]` for this binary target (to mark the
dependency as used) and does not imply runtime registration or side‑effects;
reference the fact that the binary delegates to
`test_schema_store::write_store_file` and that `avdschema` is actually used in
the library via `use avdschema::Dump as _;`/`use avdschema::Store;` so readers
understand the lint suppression intent.
In `@rust/test_schema_store/src/lib.rs`:
- Around line 51-65: The filter currently only checks for ".yml" extensions but
schema_name(path) expects filenames ending with ".schema.yml" and will panic on
mismatch; update the directory filter closure (the .filter(...) that uses
extension() and OsStr::new("yml")) to instead check the file name ends_with
".schema.yml" (e.g. use path.file_name().and_then(OsStr::to_str).is_some_and(|f|
f.ends_with(".schema.yml"))), so only conforming files reach schema_name(), or
alternatively make schema_name() return a Result/Option instead of unwrap — pick
the former to keep schema_name() as-is.
In `@tests/validation/conftest.py`:
- Around line 36-51: The subprocess.run invocation that spins up the cargo
command in tests/validation/conftest.py should include a timeout argument to
avoid indefinite hangs; update the subprocess.run call (the one invoking cargo
"run" -p "test_schema_store" --bin "build_test_schemas") to pass a reasonable
timeout (e.g., seconds as an int) and wrap it to catch subprocess.TimeoutExpired
so you can fail the test with a clear message or re-raise a descriptive
exception; ensure you reference the same subprocess.run call and handle
TimeoutExpired to provide a helpful error instead of letting the test suite
hang.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e3033d82-d7d3-4e61-8845-7649a28dd533
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.github/workflows/release.ymlCargo.tomlpyavd_utils/THIRD_PARTY_LICENSES.txtpyavd_utils/validation.pyipyproject.tomlrust/avdschema/src/schema/dict.rsrust/pyvalidation/benches/performance.rsrust/pyvalidation/src/lib.rsrust/test_schema_store/Cargo.tomlrust/test_schema_store/src/bin/build_test_schemas.rsrust/test_schema_store/src/lib.rsrust/test_schema_store/test_schemas/README.mdrust/test_schema_store/test_schemas/avd_design.schema.ymlrust/test_schema_store/test_schemas/eos_config.schema.ymlrust/test_schema_store/test_schemas/schema_features.schema.ymltests/validation/conftest.pytests/validation/test_get_validated_data.pytests/validation/test_validate_json.py
💤 Files with no reviewable changes (2)
- pyavd_utils/validation.pyi
- Cargo.toml
|
|
||
| use std::path::PathBuf; | ||
|
|
||
| use avdschema as _; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if removing the avdschema import causes build or runtime issues
# Search for any dynamic registration or lazy_static patterns in avdschema
echo "=== Checking for registration patterns in avdschema ==="
rg -n "inventory::|ctor::|lazy_static!" rust/avdschema/
# Check if binary builds without the import (test compilation only, don't modify)
echo -e "\n=== Checking what symbols the binary imports from avdschema ==="
rg -n "avdschema::" rust/test_schema_store/src/bin/build_test_schemas.rsRepository: aristanetworks/pyavd-utils
Length of output: 191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target binary around the import
echo "=== build_test_schemas.rs (context) ==="
sed -n '1,120p' rust/test_schema_store/src/bin/build_test_schemas.rs
# Confirm whether the crate name is imported anywhere else
echo -e "\n=== Other avdschema imports in rust/ ==="
rg -n "use\s+avdschema\s+as\s+_" rust || true
rg -n "extern\s+crate\s+avdschema" rust || true
# Search in the avdschema crate for common side-effect registration entrypoints
echo -e "\n=== Side-effect / registration patterns in rust/avdschema/ ==="
rg -n "inventory::|ctor::|lazy_static!|linkme::|distributed_slice!|register_.*|init_.*|side_effect|#[[]used|used\]" rust/avdschema/ || true
# Check for any re-exported module paths that might be used only for side effects
echo -e "\n=== avdschema:: usage within the binary ==="
rg -n "avdschema::" rust/test_schema_store/src/bin/build_test_schemas.rs || true
# Show the relevant Cargo.toml dependencies
echo -e "\n=== Cargo.toml files referencing avdschema ==="
rg -n "avdschema" rust/test_schema_store/Cargo.toml rust/**/Cargo.toml || trueRepository: aristanetworks/pyavd-utils
Length of output: 2422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect avdschema crate entrypoints
echo "=== rust/avdschema/src/lib.rs (top) ==="
ls -la rust/avdschema/src || true
if [ -f rust/avdschema/src/lib.rs ]; then
sed -n '1,200p' rust/avdschema/src/lib.rs
fi
# Look for side-effect / registration related constructs (no broken regex)
echo -e "\n=== Search for registration / ctor / inventory usage in rust/avdschema ==="
rg -n -S -e "inventory::" -e "ctor::" -e "lazy_static!" -e "linkme::" -e "distributed_slice!" \
-e "register_" -e "init_" -e "side_effect" -e "#\[ctor" -e "#\[distributed_slice" rust/avdschema/ || true
# Search for proc-macro style registration that might rely on crate linking
echo -e "\n=== Search for macros or derive patterns that suggest global registries ==="
rg -n -S -e "distributed_slice!" -e "inventory::collect!" -e "inventory::submit!" -e "linkme::distributed_slice" \
-e "OnceCell|once_cell" -e "lazy_static" rust/avdschema/ || true
# Confirm the feature enabling dump/load files for this binary
echo -e "\n=== rust/test_schema_store/Cargo.toml dependency on avdschema ==="
sed -n '1,120p' rust/test_schema_store/Cargo.tomlRepository: aristanetworks/pyavd-utils
Length of output: 3235
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate write_store_file implementation and inspect for avdschema usage
echo "=== Find write_store_file in rust/test_schema_store ==="
rg -n "fn\s+write_store_file|write_store_file\(" rust/test_schema_store/src || true
# Show the lib.rs (and any likely modules) around write_store_file
echo -e "\n=== Candidate files for write_store_file ==="
rg -n "pub\s+fn\s+write_store_file|write_store_file" rust/test_schema_store/src -S || true
# Print those files with line numbers (keep output small by limiting ranges)
python3 - <<'PY'
import subprocess, re, os, textwrap, json, sys, pathlib
root="."
# Collect filenames containing "write_store_file"
out=subprocess.check_output(["rg","-l","write_store_file","rust/test_schema_store/src"], text=True).splitlines()
print("\n".join(out))
PYRepository: aristanetworks/pyavd-utils
Length of output: 619
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== rust/test_schema_store/src/lib.rs (write_store_file area) ==="
sed -n '1,120p' rust/test_schema_store/src/lib.rsRepository: aristanetworks/pyavd-utils
Length of output: 2067
Document why use avdschema as _; is kept in build_test_schemas (lint suppression)
The binary has #![deny(unused_crate_dependencies)] but doesn’t reference avdschema directly (it only calls test_schema_store::write_store_file); avdschema is used in test_schema_store’s library (use avdschema::Dump as _;, use avdschema::Store;). The use avdschema as _; on line 8 is therefore likely just to mark the dependency as used for this target—so add a short comment to reflect that intent and avoid implying runtime registration/side-effect behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/test_schema_store/src/bin/build_test_schemas.rs` at line 8, Add a brief
comment above the standalone `use avdschema as _;` in build_test_schemas.rs
explaining it is present only to satisfy `#![deny(unused_crate_dependencies)]`
for this binary target (to mark the dependency as used) and does not imply
runtime registration or side‑effects; reference the fact that the binary
delegates to `test_schema_store::write_store_file` and that `avdschema` is
actually used in the library via `use avdschema::Dump as _;`/`use
avdschema::Store;` so readers understand the lint suppression intent.
Comment on lines
+51
to
65
| .filter(|path| { | ||
| path.extension() | ||
| .is_some_and(|extension| extension == OsStr::new("yml")) | ||
| }) | ||
| .map(|schema_path| (schema_name(&schema_path), schema_path)) | ||
| .collect() | ||
| } | ||
|
|
||
| fn schema_name(path: &Path) -> String { | ||
| path.file_name() | ||
| .and_then(OsStr::to_str) | ||
| .and_then(|filename| filename.strip_suffix(".schema.yml")) | ||
| .unwrap() | ||
| .to_owned() | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: Filter and name extraction logic mismatch will panic on non-conforming files.
The directory filter (lines 51-54) accepts any file with a .yml extension, but schema_name() (lines 59-65) requires filenames to end with .schema.yml and panics via unwrap() if they don't. A stray file like notes.yml in test_schemas/ would pass the filter but crash at line 63.
🔧 Fix: Align filter to match expected suffix
fn read_test_schema_paths() -> HashMap<String, PathBuf> {
std::fs::read_dir(TEST_SCHEMA_DIR)
.unwrap()
.filter_map(Result::ok)
.map(|entry| entry.path())
.filter(|path| {
- path.extension()
- .is_some_and(|extension| extension == OsStr::new("yml"))
+ path.file_name()
+ .and_then(OsStr::to_str)
+ .is_some_and(|name| name.ends_with(".schema.yml"))
})
.map(|schema_path| (schema_name(&schema_path), schema_path))
.collect()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .filter(|path| { | |
| path.extension() | |
| .is_some_and(|extension| extension == OsStr::new("yml")) | |
| }) | |
| .map(|schema_path| (schema_name(&schema_path), schema_path)) | |
| .collect() | |
| } | |
| fn schema_name(path: &Path) -> String { | |
| path.file_name() | |
| .and_then(OsStr::to_str) | |
| .and_then(|filename| filename.strip_suffix(".schema.yml")) | |
| .unwrap() | |
| .to_owned() | |
| } | |
| .filter(|path| { | |
| path.file_name() | |
| .and_then(OsStr::to_str) | |
| .is_some_and(|name| name.ends_with(".schema.yml")) | |
| }) | |
| .map(|schema_path| (schema_name(&schema_path), schema_path)) | |
| .collect() | |
| } | |
| fn schema_name(path: &Path) -> String { | |
| path.file_name() | |
| .and_then(OsStr::to_str) | |
| .and_then(|filename| filename.strip_suffix(".schema.yml")) | |
| .unwrap() | |
| .to_owned() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/test_schema_store/src/lib.rs` around lines 51 - 65, The filter currently
only checks for ".yml" extensions but schema_name(path) expects filenames ending
with ".schema.yml" and will panic on mismatch; update the directory filter
closure (the .filter(...) that uses extension() and OsStr::new("yml")) to
instead check the file name ends_with ".schema.yml" (e.g. use
path.file_name().and_then(OsStr::to_str).is_some_and(|f|
f.ends_with(".schema.yml"))), so only conforming files reach schema_name(), or
alternatively make schema_name() return a Result/Option instead of unwrap — pick
the former to keep schema_name() as-is.
Comment on lines
+36
to
+51
| subprocess.run( # noqa: S603 | ||
| [ | ||
| cargo, | ||
| "run", | ||
| "--locked", | ||
| "-p", | ||
| "test_schema_store", | ||
| "--bin", | ||
| "build_test_schemas", | ||
| "--", | ||
| "--output", | ||
| str(output_file), | ||
| ], | ||
| check=True, | ||
| cwd=repo_root, | ||
| ) |
Contributor
There was a problem hiding this comment.
Add timeout to subprocess.run to prevent indefinite hangs.
The subprocess.run call lacks a timeout parameter. If the cargo build hangs or takes unexpectedly long, the test suite will hang indefinitely, potentially blocking CI pipelines.
⏱️ Proposed fix to add timeout
subprocess.run( # noqa: S603
[
cargo,
"run",
"--locked",
"-p",
"test_schema_store",
"--bin",
"build_test_schemas",
"--",
"--output",
str(output_file),
],
check=True,
cwd=repo_root,
+ timeout=300, # 5 minutes should be sufficient for building the schema store
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/validation/conftest.py` around lines 36 - 51, The subprocess.run
invocation that spins up the cargo command in tests/validation/conftest.py
should include a timeout argument to avoid indefinite hangs; update the
subprocess.run call (the one invoking cargo "run" -p "test_schema_store" --bin
"build_test_schemas") to pass a reasonable timeout (e.g., seconds as an int) and
wrap it to catch subprocess.TimeoutExpired so you can fail the test with a clear
message or re-raise a descriptive exception; ensure you reference the same
subprocess.run call and handle TimeoutExpired to provide a helpful error instead
of letting the test suite hang.
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
gmuloc
marked this pull request as draft
June 17, 2026 07:32
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a test schema store in the crate of the same name, which will be used for all tests across both Rust and Python.
This test schema exercises all AVD schema types and fields.
The schema will be uploaded to releases as an artifact, but the main use case is to compile the binary and export it with that.
Next step is to figure out a good way to get this binary into the AVD workflows. Maybe adding it to pyavd-utils python package.
Summary by CodeRabbit
Release Notes
New Features
Chores
avd_design,eos_config,schema_features) to improve validation testing coverage.